feat(sidebar): show loading state when initially loading connections COMPASS-8876 #6710
Conversation
|
|
||
| export function getInitialConnectionsStateForConnectionInfos( | ||
| connectionInfos: ConnectionInfo[] = [] | ||
| connectionInfos: ConnectionInfo[] | null |
There was a problem hiding this comment.
Probably should've been like that from the beginning instead of falling back on empty array and building logic around it, but it's only used in tests so have no real effect on anything until now
gribnoysup
force-pushed
the
compass-8876-initial-loading-state-for-the-sidebar
branch
from
9ade8cc to
e13c3d4
Compare
| </div> | ||
| {connections.length > 0 && ( | ||
| <> | ||
| <NavigationItemsFilter |
There was a problem hiding this comment.
I'll double-check with design as I couldn't find this explicitly mentioned, but in designs we show search bar while loading, so it would be weird to yank it out when we finished, it's a noticeable and annoying visual jump, so I'm continuing to show it, just similarly disabled as when loading
There was a problem hiding this comment.
[no action] sgtm. I was going to suggest testing by deliberately hanging the load indefinitely and just see how many things you can click in the UI that you shouldn't be able to click. sounds like you experimented with that which is good!
There was a problem hiding this comment.
Can definitely extend the test I added to do that to some extent, good suggestion, thanks!
| * the connections | ||
| */ | ||
| connections?: ConnectionInfo[]; | ||
| connections?: ConnectionInfo[] | null; |
There was a problem hiding this comment.
[nonblocking] think about whether you want "null" to have different semantics from "missing" or "empty." It's a common source of bugs that the meaning of null gets overloaded and it turns up to mean different things in a couple different layers. In this case you can make the type ConnectionInfo[] | no-preload to force English meaning to appear in the code when you make the "null" choice.
There was a problem hiding this comment.
Yeah, that's fair, I'll switch it for tests helper to be something like that. Main reason I'm not putting much effort into this, is that it's literally two test cases that want this to be disabled, but it's not a great reason to not to put a bit more effort into it, so thanks for holding me accountable 😄
| onFailToLoadConnections: initialProps.onFailToLoadConnections, | ||
| }); | ||
| const store = configureStore( | ||
| initialProps.preloadStorageConnectionInfos ?? null, |
There was a problem hiding this comment.
[nonblocking] similar to the above point, is there a way we can better help the reader keep track of what null/undef mean as they read through different parts of the code?
There was a problem hiding this comment.
Hmmm, honestly I think I shouldn't have changed this file at all. Undefined (optional) props are a common pattern in React and from component props perspective this should just still expect preloads to either be passed, then do a thing, or not, then do nothing in all the places, no need to make it expect null in some places, it's not really different from "no value" case here
| </div> | ||
| {connections.length > 0 && ( | ||
| <> | ||
| <NavigationItemsFilter |
There was a problem hiding this comment.
[no action] sgtm. I was going to suggest testing by deliberately hanging the load indefinitely and just see how many things you can click in the UI that you shouldn't be able to click. sounds like you experimented with that which is good!
| ); | ||
| }; | ||
|
|
||
| const placeholderListStyles = css({ |
There was a problem hiding this comment.
there was also the option of skeleton loader in leafygreen, I think Simon suggested it in the jira, did you want to try that out?
There was a problem hiding this comment.
I'll open a follow-up ticket for that. Our component that we already use in other parts of the navigation tree (and this is why I used it here) is basically proto version of the leafygreen Skeleton, we implemented it before skeleton component existed. We should switch to leafygreen version, but I don't want to make this refactoring part of this PR, I also don't want to leave two different placeholders hanging around in the codebase
…licit in test helpers; extend the test to check for disabled elements
… screen when loading connections
gribnoysup
deleted the
compass-8876-initial-loading-state-for-the-sidebar
branch
|
Merging because you mentioned that comments are non-blocking, but take a look and tell me what you think, I'm happy to follow up if needed |
This patch updates the sidebar to show the loading state with placeholders list while we're loading connections (both for web and desktop, although loading in desktop from disc is too fast to notice).
To be able to read this state I'm adding a new hook to the compass-connections + to be able to test it I'm changing the logic in the compass-testing-library to explicitly allow to skip sync preloading of connections (in most cases we want them to be preloaded, so I'm making this behavior an opt-in through passing
null)